Add device context parameter #57

vlad-nazarov · 2021-03-17T02:08:40Z

No description provided.

PetrovKP · 2021-03-17T11:55:14Z

/azp run

vlad-nazarov · 2021-03-17T16:20:33Z

/azp run

PetrovKP · 2021-03-17T19:08:01Z

/azp run

azure-pipelines · 2021-03-17T19:08:06Z

No pipelines are associated with this pull request.

PetrovKP · 2021-03-17T19:08:42Z

/azp help

azure-pipelines · 2021-03-17T19:08:46Z

Supported commands help: Get descriptions, examples and documentation about supported commands Example: help "command_name" list: List all pipelines for this repository using a comment. Example: "list" run: Run all pipelines or specific pipelines for this repository using a comment. Use this command by itself to trigger all related pipelines, or specify specific pipelines to run. Example: "run" or "run pipeline_name, pipeline_name, pipeline_name" where: Report back the Azure DevOps orgs that are related to this repository and org Example: "where" See additional documentation.

PetrovKP · 2021-03-17T19:09:03Z

/azp list

azure-pipelines · 2021-03-17T19:09:08Z

CI/CD Pipelines for this repository: IntelPython.scikit-learn_bench

PetrovKP · 2021-03-17T19:09:21Z

/azp run

azure-pipelines · 2021-03-17T19:09:29Z

Azure Pipelines successfully started running 1 pipeline(s).

PetrovKP

overall good, but need to check the performance of the cpu (so that your changes do not bring an overhead). come to me

PetrovKP · 2021-03-17T19:10:11Z

bench.py

@@ -175,7 +175,11 @@ def parse_args(parser, size=None, loop_types=(),
                        help='Dataset name')
    parser.add_argument('--no-intel-optimized', default=False, action='store_true',
                        help='Use no intel optimized version. '
-                             'Now avalible for scikit-learn benchmarks'),
+                             'Now avalible for scikit-learn benchmarks')
+    parser.add_argument('--device', default=None, type=str,


Suggested change

parser.add_argument('--device', default=None, type=str,

parser.add_argument('--device', default="host", type=str,

My understanding is that None is used to run without context. Other values specify device type for a context

ohh, then I think the host is not needed at all

PetrovKP · 2021-03-17T19:13:10Z

configs/skl_with_context_config.json

+        "data-format": ["pandas"],
+        "data-order": ["F"],
+        "dtype": ["float64"],
+        "device": ["host", "cpu", "gpu"]


What happens if I run this config on a machine without a GPU driver?

Exactly the same what happens if you try to run on a CPU wo DPC++ support - an exception. What is your suggession here?

We shall point somewhere that using this config file requires DPC++ support and GPU device on board

Ok, probably it's ok

PetrovKP · 2021-03-17T19:14:11Z

sklearn_bench/dbscan.py

+                    min_samples=params.min_samples, metric='euclidean',
+                    algorithm='auto')
+
+    # N.B. algorithm='auto' will select DAAL's brute force method when running


Suggested change

# N.B. algorithm='auto' will select DAAL's brute force method when running

# N.B. algorithm='auto' will select oneAPI Data Analytics Library (oneDAL) brute force method when running

@PetrovKP what about other files that @vlad-nazarov did not touch?

I will correct if there are such places yet

michael-smirnov · 2021-03-18T05:31:16Z

bench.py

@@ -175,7 +175,11 @@ def parse_args(parser, size=None, loop_types=(),
                        help='Dataset name')
    parser.add_argument('--no-intel-optimized', default=False, action='store_true',
                        help='Use no intel optimized version. '
-                             'Now avalible for scikit-learn benchmarks'),
+                             'Now avalible for scikit-learn benchmarks')
+    parser.add_argument('--device', default=None, type=str,


My understanding is that None is used to run without context. Other values specify device type for a context

michael-smirnov · 2021-03-18T05:31:36Z

bench.py

-                             'Now avalible for scikit-learn benchmarks'),
+                             'Now avalible for scikit-learn benchmarks')
+    parser.add_argument('--device', default=None, type=str,
+                        choices=("host", "cpu", "gpu"),


None shall be also included?

michael-smirnov · 2021-03-18T05:33:30Z

bench.py

@@ -197,6 +201,8 @@ def parse_args(parser, size=None, loop_types=(),
        except ImportError:
            print('Failed to import daal4py.sklearn.patch_sklearn.'
                  'Use stock version scikit-learn', file=sys.stderr)
+    else:
+        params.device = None


I think we should check if the device parameter is passed by the user and print a warning that it is useless in that case - for clarity

michael-smirnov · 2021-03-18T05:35:21Z

configs/skl_with_context_config.json

+        "data-format": ["pandas"],
+        "data-order": ["F"],
+        "dtype": ["float64"],
+        "device": ["host", "cpu", "gpu"]


Exactly the same what happens if you try to run on a CPU wo DPC++ support - an exception. What is your suggession here?

michael-smirnov · 2021-03-18T05:36:37Z

configs/skl_with_context_config.json

+        "data-format": ["pandas"],
+        "data-order": ["F"],
+        "dtype": ["float64"],
+        "device": ["host", "cpu", "gpu"]


We shall point somewhere that using this config file requires DPC++ support and GPU device on board

michael-smirnov · 2021-03-18T05:37:42Z

runner.py

@@ -70,6 +70,9 @@ def generate_cases(params):
    parser.add_argument('--report', default=False, action='store_true',
                        help='Create an Excel report based on benchmarks results. '
                             'Need "openpyxl" library')
+    parser.add_argument('--device', default=None, type=str,


Why the parameter is duplicated in bench.py and runner.py?

michael-smirnov · 2021-03-18T05:38:55Z

sklearn_bench/dbscan.py

+                    min_samples=params.min_samples, metric='euclidean',
+                    algorithm='auto')
+
+    # N.B. algorithm='auto' will select DAAL's brute force method when running


@PetrovKP what about other files that @vlad-nazarov did not touch?

michael-smirnov · 2021-03-19T05:55:22Z

bench.py

-                             'Now avalible for scikit-learn benchmarks'),
+                             'Now avalible for scikit-learn benchmarks')
+    parser.add_argument('--device', default='host', type=str,
+                        choices=('host', 'cpu', 'gpu'),


Why you changed the behavior? For me, using None is better to depict we are not using contexts. host, cpu and gpu are values supported to create daal4py.oneapi.sycl_context - please do not introduce confusion here.

michael-smirnov · 2021-03-19T06:00:09Z

bench.py

+            params.device = 'host'
+    else:
+        if params.device != 'host':
+            print('Device context not supported without intel optimized version',


Device context is not supported for stock scikit-learn. Please use --no-intel-optimized=False with f'--device={params.device}' parameter. Fallback to --device=None.

michael-smirnov · 2021-03-19T06:00:57Z

configs/skl_with_context_config.json

@@ -0,0 +1,77 @@
+{
+    "common": {
+        "lib": ["sklearn"],


How stock or intel version of sk is specified for this config?

Do we need to launch the stock sk in this config? Maybe just add flag no-intel-optimized before config run? I think it's better to separate patched and unpatched sk launches

Ok - probably its better to skip device != None branches for stock sklearn

Need to add support to skip these cases?

PetrovKP

merge after green CI

michael-smirnov · 2021-03-24T13:58:29Z

configs/skl_with_context_config.json

@@ -0,0 +1,77 @@
+{
+    "common": {
+        "lib": ["sklearn"],


Ok - probably its better to skip device != None branches for stock sklearn

Vladislav Nazarov added 3 commits March 17, 2021 05:01

Initial impl

1c6eefd

Add another algs

24c24f2

Fix kmeans warning

3da61ba

PetrovKP added the gpu label Mar 17, 2021

Update config

f75263e

vlad-nazarov marked this pull request as ready for review March 17, 2021 16:20

vlad-nazarov requested review from Alexsandruss and PetrovKP as code owners March 17, 2021 16:20

vlad-nazarov requested a review from michael-smirnov March 17, 2021 16:20

PetrovKP reviewed Mar 17, 2021

View reviewed changes

michael-smirnov reviewed Mar 18, 2021

View reviewed changes

Apply comments and fix pep

9653d96

michael-smirnov suggested changes Mar 19, 2021

View reviewed changes

Fix device args

25b09b4

PetrovKP approved these changes Mar 23, 2021

View reviewed changes

Fix pep8

3f9c35d

michael-smirnov approved these changes Mar 24, 2021

View reviewed changes

vlad-nazarov merged commit b847226 into IntelPython:master Mar 25, 2021

vlad-nazarov deleted the dev/add_gpu_patch branch March 25, 2021 06:12

	parser.add_argument('--device', default=None, type=str,
	parser.add_argument('--device', default="host", type=str,

	# N.B. algorithm='auto' will select DAAL's brute force method when running
	# N.B. algorithm='auto' will select oneAPI Data Analytics Library (oneDAL) brute force method when running

Add device context parameter #57

Add device context parameter #57

Uh oh!

Conversation

vlad-nazarov commented Mar 17, 2021

Uh oh!

PetrovKP commented Mar 17, 2021

Uh oh!

vlad-nazarov commented Mar 17, 2021

Uh oh!

PetrovKP commented Mar 17, 2021

Uh oh!

azure-pipelines bot commented Mar 17, 2021

Uh oh!

PetrovKP commented Mar 17, 2021

Uh oh!

azure-pipelines bot commented Mar 17, 2021

Uh oh!

PetrovKP commented Mar 17, 2021

Uh oh!

azure-pipelines bot commented Mar 17, 2021

Uh oh!

PetrovKP commented Mar 17, 2021

Uh oh!

azure-pipelines bot commented Mar 17, 2021

Uh oh!

PetrovKP left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

PetrovKP left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!